Implement MNES Telnet Option Proxying - #208
Open
nschimme wants to merge 5 commits into
Open
Conversation
- Added support for Mud New-Environ Standard (MNES) protocol in AbstractTelnet. - Implemented client-aware proxying: MudTelnet delays WILL response until the user client supports MNES. - MudTelnet automatically handles IPADDRESS (client's real IP) and MTTS (MMapper capabilities bitvector). - ClientTelnet provides sane defaults for its MNES implementation. - UserTelnet synchronizes character encoding based on received MNES variables (MTTS, CHARSET). - Extended AbstractSocket to provide peerAddress() for IP reporting. - Improved telnetSubnegName to handle value conflicts between different options. - Fixed a bug in the NEW-ENVIRON parser that incorrectly handled "Send All" requests.
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Reviewer's GuideImplements MNES/NEW-ENVIRON-aware telnet proxying end-to-end: adds NEW-ENVIRON option support and parsing in AbstractTelnet, wires it through UserTelnet, MudTelnet, Proxy, and ClientTelnet, reports MTTS/IPADDRESS and sensible client defaults, synchronizes charset based on MNES variables, and improves telnet subnegotiation logging for overlapping subneg codes. Sequence diagram for NEW-ENVIRON negotiation gating between user and MUDsequenceDiagram
actor UserClient
participant UserTelnet
participant Proxy
participant MudTelnet
participant MudServer
UserClient->>UserTelnet: IAC WILL NEW-ENVIRON
UserTelnet->>UserTelnet: requestTelnetOption(TN_DO, OPT_NEW_ENVIRON)
UserTelnet-->>UserClient: IAC DO NEW-ENVIRON
UserTelnet->>Proxy: onNewEnvironNegotiated(true)
Proxy->>MudTelnet: onUserNewEnvironNegotiated(true)
MudTelnet->>MudTelnet: m_userSupportsNewEnviron = true
MudServer-->>MudTelnet: IAC DO NEW-ENVIRON
MudTelnet->>MudTelnet: virt_receiveNewEnvironDo()
MudTelnet->>MudTelnet: if m_userSupportsNewEnviron && !myOptionState[OPT_NEW_ENVIRON]
MudTelnet-->>MudServer: IAC WILL NEW-ENVIRON
MudTelnet->>MudTelnet: myOptionState[OPT_NEW_ENVIRON] = true
note over MudTelnet,MudServer: Mud only gets WILL NEW-ENVIRON after user support is known
Sequence diagram for NEW-ENVIRON SEND handling and MTTS/IPADDRESS reportingsequenceDiagram
actor UserClient
participant UserTelnet
participant Proxy
participant MudTelnet
participant MudServer
MudServer-->>MudTelnet: SB NEW-ENVIRON SEND MTTS, IPADDRESS SE
MudTelnet->>MudTelnet: virt_receiveNewEnvironSend(vars, userVars)
alt user does not support NEW-ENVIRON
MudTelnet->>Proxy: onRelayNewEnvironSendFromMudToUser(vars, userVars)
Proxy->>UserTelnet: onRelayNewEnvironSend(vars, userVars)
UserTelnet-->>UserClient: SB NEW-ENVIRON SEND MTTS, IPADDRESS SE
else user supports NEW-ENVIRON
MudTelnet->>MudTelnet: sendAll = vars.isEmpty() && userVars.isEmpty()
MudTelnet->>MudTelnet: detect MTTS/IPADDRESS requested
MudTelnet->>Proxy: onGetPeerAddress()
Proxy->>Proxy: getUserSocketAddress()
Proxy-->>MudTelnet: user IP (AbstractSocket::peerAddress)
MudTelnet->>MudServer: SB NEW-ENVIRON IS MTTS=653, IPADDRESS=<user IP> SE
MudTelnet->>Proxy: onRelayNewEnvironSendFromMudToUser(vars, userVars)
Proxy->>UserTelnet: onRelayNewEnvironSend(vars, userVars)
UserTelnet-->>UserClient: SB NEW-ENVIRON SEND MTTS, IPADDRESS SE
UserClient-->>UserTelnet: SB NEW-ENVIRON IS ... SE
UserTelnet->>UserTelnet: virt_receiveNewEnvironIs(vars, userVars)
UserTelnet->>Proxy: onRelayNewEnvironIsFromUserToMud(vars, userVars)
Proxy->>MudTelnet: onRelayNewEnvironIs(vars, userVars)
MudTelnet-->>MudServer: SB NEW-ENVIRON IS (relayed user vars) SE
end
Sequence diagram for charset synchronization via NEW-ENVIRON MTTS/CHARSETsequenceDiagram
actor UserClient
participant UserTelnet
participant MudTelnet
participant MudServer
rect rgb(235, 245, 255)
note over UserClient,UserTelnet: Client announces capabilities and charset
UserClient-->>UserTelnet: SB NEW-ENVIRON IS MTTS=..., CHARSET=... SE
UserTelnet->>UserTelnet: virt_receiveNewEnvironIs(vars, userVars)
UserTelnet->>UserTelnet: if MTTS bit UTF-8 set
UserTelnet->>UserTelnet: setEncodingForName(ENCODING_UTF_8)
UserTelnet->>UserTelnet: if CHARSET present
UserTelnet->>UserTelnet: setEncodingForName(CHARSET)
UserTelnet->>MudTelnet: onRelayNewEnvironIs(vars, userVars)
MudTelnet-->>MudServer: SB NEW-ENVIRON IS (relayed client vars) SE
end
rect rgb(245, 235, 255)
note over MudServer,MudTelnet: Server negotiates TERMINAL_TYPE / CHARSET
MudServer-->>MudTelnet: SB CHARSET ACCEPTED <charset> SE
MudTelnet->>MudTelnet: sendCharsetAccepted(characterSet)
MudTelnet->>MudTelnet: if myOptionState[OPT_NEW_ENVIRON]
MudTelnet->>MudServer: SB NEW-ENVIRON INFO CHARSET=<charset> SE
MudServer-->>MudTelnet: SB TERMINAL-TYPE IS <term> SE
MudTelnet->>MudTelnet: virt_receiveTerminalType(terminalType)
MudTelnet->>MudTelnet: if myOptionState[OPT_NEW_ENVIRON]
MudTelnet->>MudServer: SB NEW-ENVIRON INFO TERMINAL_TYPE=<term> SE
end
Updated class diagram for telnet NEW-ENVIRON support and socket IP reportingclassDiagram
class AbstractTelnet {
<<abstract>>
+static uint8_t OPT_NEW_ENVIRON
+static uint8_t TNSB_VAR
+static uint8_t TNSB_VAL
+static uint8_t TNSB_ESC
+static uint8_t TNSB_INFO
+static uint8_t TNSB_USERVAR
-TelnetTextCodec m_textCodec
-TelnetTermTypeBytes m_termType
+void setTerminalType(TelnetTermTypeBytes terminalType)
+void setEncodingForName(string name)
+CharacterEncodingEnum getEncoding() const
#void sendNewEnvironIs(QMap~RawBytes,RawBytes~ vars, QMap~RawBytes,RawBytes~ userVars)
#void sendNewEnvironInfo(QMap~RawBytes,RawBytes~ vars, QMap~RawBytes,RawBytes~ userVars)
#void sendNewEnvironSend(QList~RawBytes~ vars, QList~RawBytes~ userVars)
+virtual void virt_receiveNewEnvironSend(QList~RawBytes~ vars, QList~RawBytes~ userVars)
+virtual void virt_receiveNewEnvironIs(QMap~RawBytes,RawBytes~ vars, QMap~RawBytes,RawBytes~ userVars)
+virtual void virt_receiveNewEnvironInfo(QMap~RawBytes,RawBytes~ vars, QMap~RawBytes,RawBytes~ userVars)
+virtual void virt_receiveNewEnvironDo()
+virtual void virt_receiveNewEnvironWill()
+virtual void virt_receiveNewEnvironWont()
}
class UserTelnetOutputs {
+void onRelayTermTypeFromUserToMud(TelnetTermTypeBytes bytes)
+void onRelayNewEnvironIsFromUserToMud(QMap~RawBytes,RawBytes~ vars, QMap~RawBytes,RawBytes~ userVars)
+void onRelayNewEnvironInfoFromUserToMud(QMap~RawBytes,RawBytes~ vars, QMap~RawBytes,RawBytes~ userVars)
+void onNewEnvironNegotiated(bool supported)
..virtual..
#virtual void virt_onRelayTermTypeFromUserToMud(TelnetTermTypeBytes bytes)
#virtual void virt_onRelayNewEnvironIsFromUserToMud(QMap~RawBytes,RawBytes~ vars, QMap~RawBytes,RawBytes~ userVars)
#virtual void virt_onRelayNewEnvironInfoFromUserToMud(QMap~RawBytes,RawBytes~ vars, QMap~RawBytes,RawBytes~ userVars)
#virtual void virt_onNewEnvironNegotiated(bool supported)
}
class UserTelnet {
-UserTelnetOutputs &m_outputs
+void onRelayNewEnvironSend(QList~RawBytes~ vars, QList~RawBytes~ userVars)
+void virt_receiveNewEnvironSend(QList~RawBytes~ vars, QList~RawBytes~ userVars)
+void virt_receiveNewEnvironIs(QMap~RawBytes,RawBytes~ vars, QMap~RawBytes,RawBytes~ userVars)
+void virt_receiveNewEnvironInfo(QMap~RawBytes,RawBytes~ vars, QMap~RawBytes,RawBytes~ userVars)
+void virt_receiveNewEnvironDo()
+void virt_receiveNewEnvironWill()
+void virt_receiveNewEnvironWont()
}
class MudTelnetOutputs {
+void onRelayNewEnvironSendFromMudToUser(QList~RawBytes~ vars, QList~RawBytes~ userVars)
+QString onGetPeerAddress() const
..virtual..
#virtual void virt_onRelayNewEnvironSendFromMudToUser(QList~RawBytes~ vars, QList~RawBytes~ userVars)
#virtual QString virt_onGetPeerAddress() const
}
class MudTelnet {
-MudTelnetOutputs &m_outputs
-bool m_userSupportsNewEnviron
+void virt_receiveNewEnvironSend(QList~RawBytes~ vars, QList~RawBytes~ userVars)
+void virt_receiveNewEnvironIs(QMap~RawBytes,RawBytes~ vars, QMap~RawBytes,RawBytes~ userVars)
+void virt_receiveNewEnvironInfo(QMap~RawBytes,RawBytes~ vars, QMap~RawBytes,RawBytes~ userVars)
+void virt_receiveNewEnvironDo()
+void onRelayNewEnvironIs(QMap~RawBytes,RawBytes~ vars, QMap~RawBytes,RawBytes~ userVars)
+void onRelayNewEnvironInfo(QMap~RawBytes,RawBytes~ vars, QMap~RawBytes,RawBytes~ userVars)
+void onUserNewEnvironNegotiated(bool supported)
}
class ClientTelnet {
+void virt_receiveNewEnvironSend(QList~RawBytes~ vars, QList~RawBytes~ userVars)
}
class Proxy {
-std::unique_ptr~UserTelnet~ m_userTelnet
-std::unique_ptr~MudTelnet~ m_mudTelnet
-std::unique_ptr~AbstractSocket~ m_userSocket
+QString getUserSocketAddress() const
}
class AbstractSocket {
<<abstract>>
+bool isConnected() const
+QString peerAddress() const
..virtual..
#virtual bool virt_isConnected() const
#virtual QString virt_peerAddress() const
}
class TcpSocket {
+bool virt_isConnected() const
+QString virt_peerAddress() const
}
class VirtualSocket {
+bool virt_isConnected() const
+QString virt_peerAddress() const
}
class TaggedBytes {
+bool operator==(TaggedBytes a, TaggedBytes b)
+bool operator!=(TaggedBytes a, TaggedBytes b)
+bool operator<(TaggedBytes a, TaggedBytes b)
}
AbstractTelnet <|-- UserTelnet
AbstractTelnet <|-- MudTelnet
AbstractTelnet <|-- ClientTelnet
AbstractSocket <|-- TcpSocket
AbstractSocket <|-- VirtualSocket
UserTelnetOutputs <.. Proxy : implements
MudTelnetOutputs <.. Proxy : implements
Proxy o--> UserTelnet
Proxy o--> MudTelnet
Proxy o--> AbstractSocket
MudTelnet --> MudTelnetOutputs
UserTelnet --> UserTelnetOutputs
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The NEW-ENVIRON
SENDparsing appears to mishandle the "send all" case (e.g.IAC SB NEW-ENVIRON SEND IAC SE): the loop adds an empty variable tosendVars, sosendAllis never true on the receiver side; consider explicitly tracking asendAllflag whentype == TNSB_SENDand no variables are present instead of appending an empty key. - The MTTS capability bitvector is hardcoded in multiple places (653 in
MudTelnet, 525 inClientTelnet); consider centralizing these values or deriving them from a shared set of flags to avoid divergence when capabilities change.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The NEW-ENVIRON `SEND` parsing appears to mishandle the "send all" case (e.g. `IAC SB NEW-ENVIRON SEND IAC SE`): the loop adds an empty variable to `sendVars`, so `sendAll` is never true on the receiver side; consider explicitly tracking a `sendAll` flag when `type == TNSB_SEND` and no variables are present instead of appending an empty key.
- The MTTS capability bitvector is hardcoded in multiple places (653 in `MudTelnet`, 525 in `ClientTelnet`); consider centralizing these values or deriving them from a shared set of flags to avoid divergence when capabilities change.
## Individual Comments
### Comment 1
<location path="src/proxy/AbstractTelnet.cpp" line_range="1024-992" />
<code_context>
+ }
+ }
+ }
+ // Add last one
+ if (inVal) {
+ if (isUserVar) {
+ isUserVars[currentVar] = currentVal;
+ } else {
+ isVars[currentVar] = currentVal;
+ }
+ } else if (!currentVar.isEmpty() || type == TNSB_SEND) {
+ // if it's SEND and no more bytes, it means send all
+ if (type == TNSB_SEND) {
+ if (isUserVar) {
+ sendUserVars.append(currentVar);
</code_context>
<issue_to_address>
**issue (bug_risk):** Handling of empty NEW-ENVIRON SEND creates a dummy variable and breaks the `sendAll` detection downstream.
For a pure `NEW-ENVIRON SEND` (`IAC SB NEW-ENVIRON SEND IAC SE` with no VAR/USERVAR), `type == TNSB_SEND` but `currentVar` stays empty. The final `else if (!currentVar.isEmpty() || type == TNSB_SEND)` still appends an empty `RawBytes`, so `vars.isEmpty()`/`userVars.isEmpty()` become false and `sendAll` is never set in the receivers. You should only append when `!currentVar.isEmpty()`, and rely on `vars.isEmpty() && userVars.isEmpty()` to represent the "send all" case, as already done in `virt_receiveNewEnvironSend`.
</issue_to_address>
### Comment 2
<location path="src/proxy/proxy.cpp" line_range="1216-1218" />
<code_context>
return deref(m_remoteEdit);
}
+
+QString Proxy::getUserSocketAddress() const
+{
+ return m_userSocket->peerAddress();
+}
</code_context>
<issue_to_address>
**issue (bug_risk):** Dereferencing `m_userSocket` without checking for null may race with connection lifecycle.
`getUserSocketAddress()` assumes `m_userSocket` is always valid, but it’s used (via `MudTelnetOutputs::virt_onGetPeerAddress()`) in response to NEW-ENVIRON SEND. If NEW-ENVIRON arrives before a socket is fully established, or after the client disconnects, this can dereference a null `m_userSocket`. Consider guarding this with a null check (and returning an empty/placeholder address) or adding an explicit assertion, and have callers handle the “no connected user socket” case.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+1216
to
+1218
| QString Proxy::getUserSocketAddress() const | ||
| { | ||
| return m_userSocket->peerAddress(); |
There was a problem hiding this comment.
issue (bug_risk): Dereferencing m_userSocket without checking for null may race with connection lifecycle.
getUserSocketAddress() assumes m_userSocket is always valid, but it’s used (via MudTelnetOutputs::virt_onGetPeerAddress()) in response to NEW-ENVIRON SEND. If NEW-ENVIRON arrives before a socket is fully established, or after the client disconnects, this can dereference a null m_userSocket. Consider guarding this with a null check (and returning an empty/placeholder address) or adding an explicit assertion, and have callers handle the “no connected user socket” case.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #208 +/- ##
==========================================
+ Coverage 25.40% 25.67% +0.26%
==========================================
Files 519 519
Lines 43102 43462 +360
Branches 4698 4776 +78
==========================================
+ Hits 10952 11160 +208
- Misses 32150 32302 +152 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
- Fixed "Send All" parser bug in NEW-ENVIRON subnegotiation. - Implemented delayed WILL NEW-ENVIRON: MudTelnet now waits for both MUD request and user client support. - Added MTTS and CHARSET variable parsing in UserTelnet to synchronize character encoding. - Fixed various compilation errors and warnings (signedness, unused parameters, duplicate case values). - Refactored telnetSubnegName for better conflict resolution and debugging. - Added MNES unit tests in TestProxy. - Cleaned up local build artifacts.
- Completed MNES (Telnet Option 39) proxying logic with client-awareness. - Implemented character encoding synchronization via MTTS/CHARSET variables. - Resolved all compiler warnings (signedness, unused parameters, old-style casts). - Fixed duplicate case value in telnet logging utility. - Updated unit tests and CMakeLists.txt for proper verification. - Verified parsing logic with new MNES test cases.
- Restore `vars` and `userVars` parameter names in `MudTelnet` and `UserTelnet` relay methods. - Fix signedness conversion warnings in `AbstractTelnet.cpp` by correctly using `static_cast<char>` for `RawBytes::append` and avoiding it for `AppendBuffer::append`. - Resolve `vtable` warning in `TestProxy.cpp` by moving `TestTelnet` destructor out-of-line. - Fix unused parameter warnings in `TestProxy.cpp`.
nschimme
force-pushed
the
master
branch
2 times, most recently
from
April 20, 2026 18:27
e8139f3 to
c119262
Compare
nschimme
force-pushed
the
master
branch
3 times, most recently
from
May 21, 2026 23:06
ae664f2 to
bcd8fca
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This change implements the MNES (Mud New-Environ Standard) protocol as requested.
Key features:
IPADDRESSvariable.MTTSbitvector (currently set to 653: ANSI | UTF-8 | 256 COLORS | PROXY | MNES).CLIENT_NAME,CLIENT_VERSION, andCHARSET.VAR,VAL,USERVAR, andESCsequences, and correctly identifies "Send All" requests.SEND,REQUEST,MODE,EDIT).PR created automatically by Jules for task 2551435476655868122 started by @nschimme
Summary by Sourcery
Add support for Telnet NEW-ENVIRON (MNES) negotiation and proxying across client, proxy, and MUD connections, including environment variable exchange and capability reporting.
New Features:
Enhancements: